Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: add hack dockerfiles and remove unnecessary code #2

Closed
wants to merge 3 commits into from

Conversation

alias-rahil
Copy link

@alias-rahil alias-rahil commented Mar 8, 2022

  1. Remove unnecessary code
  2. New CI pipeline to update generated code and crds if required

closes #1

@viveksinghggits
Copy link
Owner

Hi @alias-rahil
can you please summarise what exactly are we trying to achieve in this PR. If this is to just close the #1 , I would rather prefer to merge the change directly. instead of running these things in CI.

@alias-rahil
Copy link
Author

alias-rahil commented Mar 9, 2022

Hey so the the CI is to make sure the crds and the generated code are always up to date. If you decided to add another field to the Kluster struct but forgot to regenerate the crds and deepcopy interfaces, the CI will add it for you. It's not required, lmk if you want me to remove the ci file.

I would still insist on retaining the hack directory though, a lot of other repos maintain bash scripts for the same purpose but I couldn't get it to work for me so instead I went with having dockerfiles.

@viveksinghggits
Copy link
Owner

Hi @alias-rahil
I am a bit hesitant about merging this with the entire change. I personally don't think that letting CI generate the code for the fields that someone added is a good idea. If someone is adding the field they would run code generator to make sure things are working as expected.
I agree, about the hack directory we should have hack directory that can be run locally as well to generate the code.

@viveksinghggits
Copy link
Owner

Also, let's make sure to add steps in README on how to use that.

@alias-rahil alias-rahil changed the title feat(ci): add hack dockerfiles and use github actions for ci feature: add hack dockerfiles Mar 13, 2022
@alias-rahil alias-rahil changed the title feature: add hack dockerfiles feature: add hack dockerfiles and remove unnecessary code Mar 13, 2022
@alias-rahil
Copy link
Author

alias-rahil commented Mar 13, 2022

@viveksinghggits
I removed the CI as requested and added docs for using the hack dockerfiles. Can you review the changes?

@viveksinghggits
Copy link
Owner

Hi Rahil,
Thanks for your PR I appreciate it. Even though I was not able to decide initially but now I think we should not merge this PR. And the reason is, people are going to come here after watching the videos and would see some of the code that was not covered in those videos.
I apologise, I should have shared this very initially itself.

@alias-rahil
Copy link
Author

We can close this now I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go mod tidy doesn't work
2 participants